feat(audio): add independent track mixing and transcript timing - #167
Open
DonIsmaelito wants to merge 8 commits into
Open
DonIsmaelito wants to merge 8 commits into
DonIsmaelito wants to merge 8 commits into
Conversation
DonIsmaelito
marked this pull request as ready for review
September 17, 2026 20:12
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 17 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="references/sources.md">
<violation number="1" location="references/sources.md:23">
P2: The catalog does not guarantee original source PTS because `source_scan.py` records FFmpeg best-effort timestamps. Change this wording to “best-effort presentation timestamps” or update the helper to emit original `pts_time` values before consumers rely on them for exact alignment.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
This was referenced Sep 17, 2026
Author
|
@cubic-dev-ai please review the latest commits again. Validate gain point pairs and raw word starts, protect transcript reports, and reject symlink output directories. Inherits the #164 fixes. Validation: 101 branch tests passed. The combined core preview passed 765 tests with two optional skips. Existing threads remain open for rechecking; this follow-up does not claim every previous finding is resolved. |
@DonIsmaelito I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
18 issues found across 18 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="helpers/prepare_source.py">
<violation number="1" location="helpers/prepare_source.py:46">
P2: When the output parent or an ancestor is a symlink, this call follows it and writes artifacts outside the requested output tree. Reject symlink components in the output path before creating the directory.</violation>
</file>
<file name="helpers/edit_clock.py">
<violation number="1" location="helpers/edit_clock.py:90">
P2: `check_partition` accepts non-integer totals that compare equal to the covered frame count, allowing invalid frame-budget values such as `True` or `10.0` through the exact-integer partition contract. Reject nonnegative totals whose type is not exactly `int` before iterating.</violation>
</file>
<file name="helpers/project_state.py">
<violation number="1" location="helpers/project_state.py:74">
P2: When the context path or its parent is a filesystem alias, `record` writes through it and can overwrite a file outside the intended project directory. Reject symlinked or multiply-linked contexts and symlinked parent directories before saving.</violation>
</file>
<file name="helpers/edit_io.py">
<violation number="1" location="helpers/edit_io.py:63">
P2: When a transcript report uses a path beneath a symlinked directory, `save_json` writes outside the requested location. `exclusive=True` protects only the final component; reject symlinked path components before creating `path.parent`.</violation>
</file>
<file name="helpers/map_transcript.py">
<violation number="1" location="helpers/map_transcript.py:24">
P2: A word entry missing `start` or `end` raises an uncaught KeyError that aborts the entire map run. The codebase already treats these fields as optional on real transcripts: pack_transcripts.py falls back with `w.get("end", w.get("start", ...))`, and render.py's `_words_in_range` does `ws = w.get("start"); we = w.get("end"); if ws is None or we is None: continue`. Use `.get()` and skip incomplete word entries here, and do the same for `w["start"]`/`o["start"]`/`w["text"]` in `compare_words`.</violation>
<violation number="2" location="helpers/map_transcript.py:58">
P2: When final ASR contains a negative or non-finite `start`, `compare_words` can emit a report or silently filter the word instead of rejecting malformed timing evidence. Validate every retained ASR start as finite and nonnegative before interval filtering and sequence comparison.</violation>
<violation number="3" location="helpers/map_transcript.py:80">
P3: `onset_delta_ms` divides the sample difference by a hard-coded 48 (48000 samples/s / 1000). The constant silently depends on the module-level SAMPLE_RATE in edit_clock.py; derive it as `SAMPLE_RATE / 1000` so the milliseconds conversion stays correct if the sample rate ever changes.</violation>
<violation number="4" location="helpers/map_transcript.py:133">
P2: When `--out` is inside a symlinked directory, `save_json` follows the parent and creates the report outside the requested output tree. Reject symlinked parent components before saving, matching the output-directory safety used by `mix_audio.build`.</violation>
<violation number="5" location="helpers/map_transcript.py:133">
P3: When `--out` already exists but is not one of the inputs, the overwrite guard passes and `save_json(..., exclusive=True)` raises an uncaught FileExistsError traceback. The guard already produces a clean `p.error` for input aliases; give an existing output file the same friendly treatment so the agent sees a message instead of a stack trace.</violation>
</file>
<file name="helpers/find_shot.py">
<violation number="1" location="helpers/find_shot.py:94">
P2: When the report’s parent directory is a symlink, this guard still allows `save_json` to write the report into the symlink target. Reject symlink components in the output parent path before searching or writing.</violation>
</file>
<file name="helpers/mix_audio.py">
<violation number="1" location="helpers/mix_audio.py:253">
P2: When `--out-dir` is a new directory below a symlinked parent, `dest.is_symlink()` is false and `mkdir(parents=True)` follows the parent, so artifacts are written through the alias. Reject symlink components in the output path before creating it.</violation>
<violation number="2" location="helpers/mix_audio.py:287">
P2: When `normalize` fails (silent mix raises "cannot normalize silent audio", or the loudnorm second pass fails), the stems and `mix.wav` are already written, `mix_report.json` is not, and the `outputs` existence precheck then permanently blocks any retry with FileExistsError until manual cleanup. Consider cleaning up the artifacts written so far (or writing them under a staging path and renaming into place) before re-raising, so a failed run does not poison the output directory without an explanation.</violation>
</file>
<file name="helpers/source_scan.py">
<violation number="1" location="helpers/source_scan.py:118">
P2: When FFmpeg emits the requested frames but then exits nonzero, `selected_frames` returns them without reporting the decode failure because `process.wait()` is ignored. Preserve and surface the natural FFmpeg exit status while distinguishing intentional termination during generator cleanup.</violation>
<violation number="2" location="helpers/source_scan.py:154">
P2: When `--out` is inside a symlinked directory, this check passes and `save_json` writes the catalog through the alias into the symlink target. Reject symlinked output-directory components before creating the report.</violation>
</file>
<file name="tests/test_source_review.py">
<violation number="1" location="tests/test_source_review.py:37">
P3: `importorskip('cv2')` skips both parametrized cases when cv2 is missing, but only `find_shot.py` imports cv2. `source_scan.py` imports only numpy/PIL (base deps), and cv2 lives in the optional `editing` extra per pyproject.toml, so this hidden skip drops the source_scan hardlink-guard regression in base test environments too. Guard only the find_shot case.</violation>
</file>
<file name="pyproject.toml">
<violation number="1" location="pyproject.toml:17">
P2: The new `test` extra (`pytest>=7`) is declared in `[project.optional-dependencies]` but `uv.lock` was not regenerated to include it. The lock's `video-use` entry lists only `animations` and `editing` under `[package.optional-dependencies]` and `provides-extras`, and neither `requires-dist` nor any `[[package]]` contains pytest, while scipy and opencv (the other extras) were locked. As a result, `uv sync --extra test --locked` / `uv run --extra test pytest` fails against the frozen lock and CI provisioning for the declared test extra breaks; the PR's claim that lockfile checks pass does not cover extras. Regenerate the lock (e.g. `uv lock --extra test` or `uv sync --extra test`) so pyproject.toml and uv.lock agree.</violation>
</file>
<file name="tests/test_audio_tracks.py">
<violation number="1" location="tests/test_audio_tracks.py:33">
P3: `test_invalid_decode_window` is silently skipped on machines without ffmpeg even though it never decodes media. The test pulls the path from the `stereo` fixture, whose `pytest.skip("FFmpeg required")` guard runs before the test, but `decode_window` raises its `ValueError` during input validation (type/range guard) before it ever opens or decodes the file. Use a literal path string and drop the fixture so pure input-validation coverage does not depend on ffmpeg availability.</violation>
<violation number="2" location="tests/test_audio_tracks.py:105">
P3: A gain point pair whose gain value is non-numeric, e.g. `[[0, None]]`, is not covered by the new malformed-point tests, and the implementation raises `TypeError` for it instead of `ValueError`. `gain_envelope` runs `math.isfinite(float(p[1]))` only after the structure check, so `float(None)` throws `TypeError` (unhandled, non-actionable), while the test comment promises "fail with actionable errors instead of indexing crashes". Add rows such as `[[0, None]]` / `[[0, "abc"]]` to the parametrize list and guard gain values against non-numeric types before the `float()` conversion so all malformed pairs fail as `ValueError`.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| "tonemap=tonemap=mobius:desat=0", | ||
| "zscale=transfer=bt709:matrix=bt709:range=limited", | ||
| ] | ||
| out.parent.mkdir(parents=True, exist_ok=True) |
There was a problem hiding this comment.
P2: When the output parent or an ancestor is a symlink, this call follows it and writes artifacts outside the requested output tree. Reject symlink components in the output path before creating the directory.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/prepare_source.py, line 46:
<comment>When the output parent or an ancestor is a symlink, this call follows it and writes artifacts outside the requested output tree. Reject symlink components in the output path before creating the directory.</comment>
<file context>
@@ -0,0 +1,112 @@
+ "tonemap=tonemap=mobius:desat=0",
+ "zscale=transfer=bt709:matrix=bt709:range=limited",
+ ]
+ out.parent.mkdir(parents=True, exist_ok=True)
+ args = [
+ "ffmpeg",
</file context>
Suggested change
| out.parent.mkdir(parents=True, exist_ok=True) | |
| parent = out.parent | |
| while parent != parent.parent: | |
| if parent.is_symlink(): | |
| raise ValueError("source output directory cannot be a symlink") | |
| parent = parent.parent | |
| out.parent.mkdir(parents=True, exist_ok=True) |
| # reject shot intervals with gaps overlaps or an incorrect total | ||
| def check_partition(shots, total): | ||
| """Reject shot intervals with gaps overlaps or an incorrect total.""" | ||
| cursor = 0 |
There was a problem hiding this comment.
P2: check_partition accepts non-integer totals that compare equal to the covered frame count, allowing invalid frame-budget values such as True or 10.0 through the exact-integer partition contract. Reject nonnegative totals whose type is not exactly int before iterating.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/edit_clock.py, line 90:
<comment>`check_partition` accepts non-integer totals that compare equal to the covered frame count, allowing invalid frame-budget values such as `True` or `10.0` through the exact-integer partition contract. Reject nonnegative totals whose type is not exactly `int` before iterating.</comment>
<file context>
@@ -0,0 +1,104 @@
+# reject shot intervals with gaps overlaps or an incorrect total
+def check_partition(shots, total):
+ """Reject shot intervals with gaps overlaps or an incorrect total."""
+ cursor = 0
+ for shot in shots:
+ start, end = shot["start_frame"], shot["end_frame"]
</file context>
| } | ||
| data["artifacts"][ident] = entry | ||
| validate(data) | ||
| save_json(context, data) |
There was a problem hiding this comment.
P2: When the context path or its parent is a filesystem alias, record writes through it and can overwrite a file outside the intended project directory. Reject symlinked or multiply-linked contexts and symlinked parent directories before saving.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/project_state.py, line 74:
<comment>When the context path or its parent is a filesystem alias, `record` writes through it and can overwrite a file outside the intended project directory. Reject symlinked or multiply-linked contexts and symlinked parent directories before saving.</comment>
<file context>
@@ -0,0 +1,133 @@
+ }
+ data["artifacts"][ident] = entry
+ validate(data)
+ save_json(context, data)
+
+
</file context>
| def save_json(path, data, *, exclusive=False): | ||
| """Write readable JSON while rejecting nonfinite measurement values.""" | ||
| path = Path(path) | ||
| path.parent.mkdir(parents=True, exist_ok=True) |
There was a problem hiding this comment.
P2: When a transcript report uses a path beneath a symlinked directory, save_json writes outside the requested location. exclusive=True protects only the final component; reject symlinked path components before creating path.parent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/edit_io.py, line 63:
<comment>When a transcript report uses a path beneath a symlinked directory, `save_json` writes outside the requested location. `exclusive=True` protects only the final component; reject symlinked path components before creating `path.parent`.</comment>
<file context>
@@ -0,0 +1,111 @@
+def save_json(path, data, *, exclusive=False):
+ """Write readable JSON while rejecting nonfinite measurement values."""
+ path = Path(path)
+ path.parent.mkdir(parents=True, exist_ok=True)
+ payload = json.dumps(data, indent=2, allow_nan=False) + "\n"
+ with path.open("x" if exclusive else "w") as stream:
</file context>
Suggested change
| path.parent.mkdir(parents=True, exist_ok=True) | |
| if path.is_symlink() or any( | |
| parent.is_symlink() for parent in (path.parent, *path.parent.parents) | |
| ): | |
| raise ValueError("JSON output path cannot use symlinks") | |
| path.parent.mkdir(parents=True, exist_ok=True) |
| if c["role"] == "voice" | ||
| ] | ||
| result = compare_words(manifest["words"], load_json(a.final_asr), intervals) | ||
| save_json(a.out, result, exclusive=True) |
There was a problem hiding this comment.
P2: When --out is inside a symlinked directory, save_json follows the parent and creates the report outside the requested output tree. Reject symlinked parent components before saving, matching the output-directory safety used by mix_audio.build.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/map_transcript.py, line 133:
<comment>When `--out` is inside a symlinked directory, `save_json` follows the parent and creates the report outside the requested output tree. Reject symlinked parent components before saving, matching the output-directory safety used by `mix_audio.build`.</comment>
<file context>
@@ -0,0 +1,137 @@
+ if c["role"] == "voice"
+ ]
+ result = compare_words(manifest["words"], load_json(a.final_asr), intervals)
+ save_json(a.out, result, exclusive=True)
+
+
</file context>
| # existing aliases are rejected before either source command reads media | ||
| @pytest.mark.parametrize('helper', ['source_scan.py', 'find_shot.py']) | ||
| def test_hardlink_report_preserves_input(tmp_path, helper): | ||
| pytest.importorskip('cv2') |
There was a problem hiding this comment.
P3: importorskip('cv2') skips both parametrized cases when cv2 is missing, but only find_shot.py imports cv2. source_scan.py imports only numpy/PIL (base deps), and cv2 lives in the optional editing extra per pyproject.toml, so this hidden skip drops the source_scan hardlink-guard regression in base test environments too. Guard only the find_shot case.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_source_review.py, line 37:
<comment>`importorskip('cv2')` skips both parametrized cases when cv2 is missing, but only `find_shot.py` imports cv2. `source_scan.py` imports only numpy/PIL (base deps), and cv2 lives in the optional `editing` extra per pyproject.toml, so this hidden skip drops the source_scan hardlink-guard regression in base test environments too. Guard only the find_shot case.</comment>
<file context>
@@ -0,0 +1,77 @@
+# existing aliases are rejected before either source command reads media
+@pytest.mark.parametrize('helper', ['source_scan.py', 'find_shot.py'])
+def test_hardlink_report_preserves_input(tmp_path, helper):
+ pytest.importorskip('cv2')
+ source = tmp_path / 'source'; source.write_bytes(b'original source')
+ out = tmp_path / 'report'; os.link(source, out)
</file context>
Comment on lines
+33
to
+36
| def test_invalid_decode_window(stereo, start, count): | ||
| with pytest.raises(ValueError, match="window"): | ||
| decode_window(stereo[0], start, count) | ||
|
|
There was a problem hiding this comment.
P3: test_invalid_decode_window is silently skipped on machines without ffmpeg even though it never decodes media. The test pulls the path from the stereo fixture, whose pytest.skip("FFmpeg required") guard runs before the test, but decode_window raises its ValueError during input validation (type/range guard) before it ever opens or decodes the file. Use a literal path string and drop the fixture so pure input-validation coverage does not depend on ffmpeg availability.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_audio_tracks.py, line 33:
<comment>`test_invalid_decode_window` is silently skipped on machines without ffmpeg even though it never decodes media. The test pulls the path from the `stereo` fixture, whose `pytest.skip("FFmpeg required")` guard runs before the test, but `decode_window` raises its `ValueError` during input validation (type/range guard) before it ever opens or decodes the file. Use a literal path string and drop the fixture so pure input-validation coverage does not depend on ffmpeg availability.</comment>
<file context>
@@ -0,0 +1,108 @@
+
+# invalid sample boundaries fail before starting the media decoder
+@pytest.mark.parametrize("start,count", [(-1, 100), (0, 0), (1.5, 10), (True, 10)])
+def test_invalid_decode_window(stereo, start, count):
+ with pytest.raises(ValueError, match="window"):
+ decode_window(stereo[0], start, count)
</file context>
Suggested change
| def test_invalid_decode_window(stereo, start, count): | |
| with pytest.raises(ValueError, match="window"): | |
| decode_window(stereo[0], start, count) | |
| @pytest.mark.parametrize("start,count", [(-1, 100), (0, 0), (1.5, 10), (True, 10)]) | |
| def test_invalid_decode_window(start, count): | |
| # raises during input validation, before any ffmpeg decode runs | |
| with pytest.raises(ValueError, match="window"): | |
| decode_window("unused.wav", start, count) |
|
|
||
| # malformed envelope rows fail with actionable errors instead of indexing crashes | ||
| @pytest.mark.parametrize('points', [[[]], [[0]], [None], [1]]) | ||
| def test_review_malformed_gain_points(points): |
There was a problem hiding this comment.
P3: A gain point pair whose gain value is non-numeric, e.g. [[0, None]], is not covered by the new malformed-point tests, and the implementation raises TypeError for it instead of ValueError. gain_envelope runs math.isfinite(float(p[1])) only after the structure check, so float(None) throws TypeError (unhandled, non-actionable), while the test comment promises "fail with actionable errors instead of indexing crashes". Add rows such as [[0, None]] / [[0, "abc"]] to the parametrize list and guard gain values against non-numeric types before the float() conversion so all malformed pairs fail as ValueError.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_audio_tracks.py, line 105:
<comment>A gain point pair whose gain value is non-numeric, e.g. `[[0, None]]`, is not covered by the new malformed-point tests, and the implementation raises `TypeError` for it instead of `ValueError`. `gain_envelope` runs `math.isfinite(float(p[1]))` only after the structure check, so `float(None)` throws `TypeError` (unhandled, non-actionable), while the test comment promises "fail with actionable errors instead of indexing crashes". Add rows such as `[[0, None]]` / `[[0, "abc"]]` to the parametrize list and guard gain values against non-numeric types before the `float()` conversion so all malformed pairs fail as `ValueError`.</comment>
<file context>
@@ -0,0 +1,108 @@
+
+# malformed envelope rows fail with actionable errors instead of indexing crashes
+@pytest.mark.parametrize('points', [[[]], [[0]], [None], [1]])
+def test_review_malformed_gain_points(points):
+ from mix_audio import gain_envelope
+ with pytest.raises(ValueError, match='pairs'):
</file context>
| "onset_delta_ms": ( | ||
| seconds_to_sample(o["start"]) - p["start_sample"] | ||
| ) | ||
| / 48, |
There was a problem hiding this comment.
P3: onset_delta_ms divides the sample difference by a hard-coded 48 (48000 samples/s / 1000). The constant silently depends on the module-level SAMPLE_RATE in edit_clock.py; derive it as SAMPLE_RATE / 1000 so the milliseconds conversion stays correct if the sample rate ever changes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/map_transcript.py, line 80:
<comment>`onset_delta_ms` divides the sample difference by a hard-coded 48 (48000 samples/s / 1000). The constant silently depends on the module-level SAMPLE_RATE in edit_clock.py; derive it as `SAMPLE_RATE / 1000` so the milliseconds conversion stays correct if the sample rate ever changes.</comment>
<file context>
@@ -0,0 +1,137 @@
+ "onset_delta_ms": (
+ seconds_to_sample(o["start"]) - p["start_sample"]
+ )
+ / 48,
+ }
+ for p, o in zip(left[a:b], right[c:d])
</file context>
Suggested change
| / 48, | |
| / (SAMPLE_RATE / 1000), |
| if c["role"] == "voice" | ||
| ] | ||
| result = compare_words(manifest["words"], load_json(a.final_asr), intervals) | ||
| save_json(a.out, result, exclusive=True) |
There was a problem hiding this comment.
P3: When --out already exists but is not one of the inputs, the overwrite guard passes and save_json(..., exclusive=True) raises an uncaught FileExistsError traceback. The guard already produces a clean p.error for input aliases; give an existing output file the same friendly treatment so the agent sees a message instead of a stack trace.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/map_transcript.py, line 133:
<comment>When `--out` already exists but is not one of the inputs, the overwrite guard passes and `save_json(..., exclusive=True)` raises an uncaught FileExistsError traceback. The guard already produces a clean `p.error` for input aliases; give an existing output file the same friendly treatment so the agent sees a message instead of a stack trace.</comment>
<file context>
@@ -0,0 +1,137 @@
+ if c["role"] == "voice"
+ ]
+ result = compare_words(manifest["words"], load_json(a.final_asr), intervals)
+ save_json(a.out, result, exclusive=True)
+
+
</file context>
Suggested change
| save_json(a.out, result, exclusive=True) | |
| try: | |
| save_json(a.out, result, exclusive=True) | |
| except FileExistsError: | |
| p.error("output file already exists; choose a new path") |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Music should keep playing smoothly when the picture cuts, and speech should stay lined up with its captions. These helpers let the agent control music, speech and sound effects separately, and keep word timestamps aligned with the edited audio.
Builds on #164 for shared media and timing helpers.
Changes
Feature commits and review fixes, with tests alongside each feature:
references/audio-mixing.mdand expose the helpers inSKILL.md.All 101 branch tests passed, including 47 mixing/transcript cases with real FFmpeg decoding, stereo sample checks and independent master loudness measurement. CLI, comment, skill-rule and lockfile checks passed.
Limits